RSDK-1959 — Fix macos webcams disconnections cause viam-server to freeze #5492
Conversation
viambot
added
the
safe to test
viambot
added
safe to test
viambot
added
safe to test
| c.buffer.frame = nil | ||
| // Make a private copy of the previously published frame so consumers can continue to read it, | ||
| // then return the Reader-managed buffer before we kick off the next read. This avoids holding | ||
| // on to driver memory while still serving the last frame. |
There was a problem hiding this comment.
Introducing prevRelease fixes the hang. All other changes on this PR are to supplement it, to ensure best practices, and fix vulnerabilities in the code I saw along the way of fixing the hang.
viambot
added
safe to test
| } | ||
|
|
||
| // Must lock the mutex before using this function. | ||
| func (buffer *WebcamBuffer) stopBuffer() { |
There was a problem hiding this comment.
I hate how buffer is used to group together the actual buffer and the worker that fills it. I made a lot of changes to fix that naming and also deleted this helper function that is frankly a bit confusing.
| c.buffer.worker.Stop() // Calling this before locking shuts down the goroutines, and allows stopBuffer() to handle rest of the shutdown. | ||
|
|
||
| // Stop the driver and frame buffer worker | ||
| c.mu.Lock() |
There was a problem hiding this comment.
Is there a reason this shouldn't be an AlwaysRebuild resource? Seems like that may simplify things but lmk if im missing something
There was a problem hiding this comment.
I'm 90% sure it could be one. Would just be a rather big change.
| ) (video.Reader, driverutils.Driver, string, error) { | ||
| mediadevicescamera.Initialize() | ||
| if runtime.GOOS == "linux" { | ||
| // TODO(RSDK-12789): Separate discover() calls from Initialize() calls. |
There was a problem hiding this comment.
Do we need to wait on RSDK-12789 before merging? How does initialization happen on darwin?
There was a problem hiding this comment.
no, it'll work this way, just not my favorite way to "refresh" cameras by calling a function named Initialize that is only supposed to handle initialization
There was a problem hiding this comment.
How does initialization happen on darwin?
The answer to this question is complicated and I would prefer to explain it in person
| var prevRelease func() | ||
| c.mu.Lock() | ||
| if c.buffer.release != nil && c.buffer.frame != nil { | ||
| c.buffer.frame = rimage.CloneImage(c.buffer.frame) |
There was a problem hiding this comment.
So we are making a private copy on every frame here? Is perf an issue?
There was a problem hiding this comment.
good point. Let me profile and look into alternatives
viambot
added
safe to test
|
|
||
| if conf.Width != 0 && conf.Height != 0 { | ||
| if img.Bounds().Dx() != conf.Width || img.Bounds().Dy() != conf.Height { | ||
| return nil, nil, "", errors.Errorf("requested width and height (%dx%d) are not available for this webcam"+ |
There was a problem hiding this comment.
Should we just warning log here and succeed if dimensions do not exactly match?
4 participants
Tests on linux/arm64 (rpi5b)
In fact, this PR fixed an unreported linux bug too (unplug/replug is not working on linux/arm64 on the main branch).
Tests on darwin/arm64 (viam issued macbook air laptop)